Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: fix wrong type Residue -> DefinitionResidue #412

Merged
merged 9 commits into from
Dec 27, 2024

Conversation

kiyoon
Copy link
Collaborator

@kiyoon kiyoon commented Dec 16, 2024

The Residue type doesn't have altnames, so it must have been documented with the wrong type. The correct type seems to be DefinitionResidue.

I'm fixing the type issue with a type checker. Untyped code makes it vulnerable to random crashes thus making it hard to develop. Below is an example of errors that need to be addressed in the future:

image

For now, I want to keep each PR simple. I didn't fix all issues but I'm slowly trying to.

FYI: adding types is usually safe because they are ignored on runtime. Even if you pass an object with a different type by mistake, it won't error out during runtime (only in the IDE it will show an error.) It just helps the IDE to figure out what they are.

Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 66.36%. Comparing base (be4e943) to head (27eb2a7).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #412      +/-   ##
==========================================
+ Coverage   66.22%   66.36%   +0.14%     
==========================================
  Files          30       30              
  Lines        7765     7742      -23     
==========================================
- Hits         5142     5138       -4     
+ Misses       2623     2604      -19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kiyoon
Copy link
Collaborator Author

kiyoon commented Dec 27, 2024

I think we should remove setup.py and use pyproject.toml to configure the build. Ruff didn't detect the python version, thus it reported error. While we can specifically configure ruff, this means we have two locations for the configuration.

A little bit of background:

setup.py is old because it requires users to "run" the python program in order to know the spec of the package. What packages does it require? Which python version user is using to install it? Is it even compatible with the current system? Nothing can be known unless you try it and fail.

However, pyproject.toml resolves this by specifying all specs in a config file rather than python. This way, other tools like ruff can even know what the program requires etc.

@kiyoon
Copy link
Collaborator Author

kiyoon commented Dec 27, 2024

OK, I've made the change. I think you would like this.

  1. No setup.py, setup.cfg, MANIFEST.in

    • it used to be confusing how to include files in the distribution tarballs or wheel.
    • Now, you simply specify some directories and everything except .gitignored items will be included.
  2. Not generating pdb2pqr.egg-info files in the project folder.

Test building the package using

# option 1. uv (recommended)
pip install uv
uv build

# option 2. pyproject-build (slower)
pip install build
python -m build

Unpack the wheel and tarball to see what they include

pip install wheel
wheel unpack dist/pdb2pqr-3.6.2-py3-none-any.whl

pdb2pqr/structures.py Show resolved Hide resolved
setup.py Outdated Show resolved Hide resolved
@sobolevnrm sobolevnrm merged commit 15b1baf into Electrostatics:master Dec 27, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants